src: support WeakReference, diagnostics_channel and net in the snapshot by joyeecheung · Pull Request #44193 · nodejs/node

joyeecheung · 2022-08-09T18:07:19Z

The first commit comes from 44192.

The support for net isn't complete, for example there is still work to be done to make the DNS code snapshotable. But it's already alright to snapshot sockets that have never fired DNS requests and are not actively open.

Refs: #44014
Refs: #37476

nodejs-github-bot · 2022-08-09T18:07:24Z

Review requested:

@nodejs/gyp
@nodejs/net
@nodejs/startup

joyeecheung · 2022-08-09T18:12:49Z

test/parallel/test-snapshot-weak-reference.js

+}
+
+runTest(fixtures.path('snapshot', 'weak-reference.js'));
+runTest(fixtures.path('snapshot', 'weak-reference-gc.js'));


I am not 100% sure that we should test this, because technically V8 does not guarantee that the object would actually be GC'ed after a gc() call, but we already have an internal test for WeakReference and it has not come up as flaky so far so 🤷.

mcollina

lgtm

nodejs-github-bot · 2022-08-09T18:30:26Z

CI: https://ci.nodejs.org/job/node-test-pull-request/45955/

src/node_snapshotable.h

nodejs-github-bot · 2022-08-10T13:57:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/45972/

legendecas · 2022-08-10T17:04:08Z

Marking this as blocked since it depends on #44192. Please feel free to unmark it.

joyeecheung · 2022-08-11T04:30:59Z

Probably blocked on #44203 too unless we want to skip the weak reference test in the debug build..

Move util::WeakReference to a separate header and implement {de}serialization for it to be snapshotable.

nodejs-github-bot · 2022-08-18T14:51:48Z

CI: https://ci.nodejs.org/job/node-test-pull-request/46063/

joyeecheung · 2022-08-18T14:52:09Z

Rebased since #44192 and #44203 has landed

Move util::WeakReference to a separate header and implement {de}serialization for it to be snapshotable. PR-URL: #44193 Refs: #44014 Refs: #37476 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

PR-URL: #44193 Refs: #44014 Refs: #37476 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

joyeecheung · 2022-08-19T04:59:45Z

Landed in dab2057...fddc701

ruyadorno · 2022-08-23T03:01:33Z

Marking this as blocked since it depends on #44192. Please feel free to unmark it.

#44192 will need to be backported so this one will remain backport-blocked for now

Move util::WeakReference to a separate header and implement {de}serialization for it to be snapshotable. PR-URL: nodejs#44193 Refs: nodejs#44014 Refs: nodejs#37476 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

PR-URL: nodejs#44193 Refs: nodejs#44014 Refs: nodejs#37476 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

Move util::WeakReference to a separate header and implement {de}serialization for it to be snapshotable. PR-URL: nodejs#44193 Refs: nodejs#44014 Refs: nodejs#37476 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

PR-URL: nodejs#44193 Refs: nodejs#44014 Refs: nodejs#37476 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

juanarbol · 2022-09-30T03:38:58Z

Marking this as blocked since it depends on #44192. Please feel free to unmark it.

#44192 will need to be backported so this one will remain backport-blocked for now

Same thing for v16.x

Move util::WeakReference to a separate header and implement {de}serialization for it to be snapshotable. PR-URL: nodejs#44193 Refs: nodejs#44014 Refs: nodejs#37476 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

Move util::WeakReference to a separate header and implement {de}serialization for it to be snapshotable. PR-URL: #44193 Refs: #44014 Refs: #37476 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

- nodejs/node#44179 - nodejs/node#44193

* chore: bump node in DEPS to v18.13.0 * child_process: validate arguments for null bytes nodejs/node#44782 * bootstrap: merge main thread and worker thread initializations nodejs/node#44869 * module: ensure relative requires work from deleted directories nodejs/node#42384 * src: add support for externally shared js builtins nodejs/node#44000 * lib: disambiguate `native module` to `binding` nodejs/node#45673 * test: convert test-debugger-pid to async/await nodejs/node#45179 * deps: upgrade to libuv 1.44.2 nodejs/node#42340 * src: fix cppgc incompatibility in v8 nodejs/node#43521 * src: use qualified `std::move` call in node_http2 nodejs/node#45555 * build: fix env.h for cpp20 nodejs/node#45516 * test: remove experimental-wasm-threads flag nodejs/node#45074 * src: iwyu in cleanup_queue.cc nodejs/node#44983 * src: add missing include for `std::all_of` nodejs/node#45541 * deps: update ICU to 72.1 nodejs/node#45068 * chore: fixup patch indices * chore: remove errant semicolons - nodejs/node#44179 - nodejs/node#44193 * src: add support for externally shared js builtins nodejs/node#44376 * chore: add missing GN filenames * deps: update nghttp2 to 1.51.0 nodejs/node#45537 * chore: disable more Node.js snapshot tests The Snapshot feature is currently disabled * chore: disable ICU timezone tests Node.js uses a different version of ICU than Electron so they will often be out of sync. * chore: disable threadpool event tracing test Event tracing is not enabled in embedded Node.js * chore: fixup patch indices * chore: comments from review Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

* nodejs/node#47887 * nodejs/node#49684 * nodejs/node#44193

* chore: upgrade to Node.js v20 * src: allow embedders to override NODE_MODULE_VERSION nodejs/node#49279 * src: fix missing trailing , nodejs/node#46909 * src,tools: initialize cppgc nodejs/node#45704 * tools: allow passing absolute path of config.gypi in js2c nodejs/node#49162 * tools: port js2c.py to C++ nodejs/node#46997 * doc,lib: disambiguate the old term, NativeModule nodejs/node#45673 * chore: fixup Node.js BSSL tests * nodejs/node#49492 * nodejs/node#44498 * deps: upgrade to libuv 1.45.0 nodejs/node#48078 * deps: update V8 to 10.7 nodejs/node#44741 * test: use gcUntil() in test-v8-serialize-leak nodejs/node#49168 * module: make CJS load from ESM loader nodejs/node#47999 * src: make BuiltinLoader threadsafe and non-global nodejs/node#45942 * chore: address changes to CJS/ESM loading * module: make CJS load from ESM loader (nodejs/node#47999) * lib: improve esm resolve performance (nodejs/node#46652) * bootstrap: optimize modules loaded in the built-in snapshot nodejs/node#45849 * test: mark test-runner-output as flaky nodejs/node#49854 * lib: lazy-load deps in modules/run_main.js nodejs/node#45849 * url: use private properties for brand check nodejs/node#46904 * test: refactor `test-node-output-errors` nodejs/node#48992 * assert: deprecate callTracker nodejs/node#47740 * src: cast v8::Object::GetInternalField() return value to v8::Value nodejs/node#48943 * test: adapt test-v8-stats for V8 update nodejs/node#45230 * tls: ensure TLS Sockets are closed if the underlying wrap closes nodejs/node#49327 * test: deflake test-tls-socket-close nodejs/node#49575 * net: fix crash due to simultaneous close/shutdown on JS Stream Sockets nodejs/node#49400 * net: use asserts in JS Socket Stream to catch races in future nodejs/node#49400 * lib: fix BroadcastChannel initialization location nodejs/node#46864 * src: create BaseObject with node::Realm nodejs/node#44348 * src: implement DataQueue and non-memory resident Blob nodejs/node#45258 * sea: add support for V8 bytecode-only caching nodejs/node#48191 * chore: fixup patch indices * gyp: put filenames in variables nodejs/node#46965 * build: modify js2c.py into GN executable * fix: (WIP) handle string replacement of fs -> original-fs * [v20.x] backport vm-related memory fixes nodejs/node#49874 * src: make BuiltinLoader threadsafe and non-global nodejs/node#45942 * src: avoid copying string in fs_permission nodejs/node#47746 * look upon my works ye mighty and dispair * chore: patch cleanup * [api] Remove AllCan Read/Write https://chromium-review.googlesource.com/c/v8/v8/+/5006387 * fix: missing include for NODE_EXTERN * chore: fixup patch indices * fix: fail properly when js2c fails in Node.js * build: fix js2c root_gen_dir * fix: lib/fs.js -> lib/original-fs.js * build: fix original-fs file xforms * fixup! module: make CJS load from ESM loader * build: get rid of CppHeap for now * build: add patch to prevent extra fs lookup on esm load * build: greatly simplify js2c modifications Moves our original-fs modifications back into a super simple python script action, wires up the output of that action into our call to js2c * chore: update to handle moved internal/modules/helpers file * test: update @types/node test * feat: enable preventing cppgc heap creation * feat: optionally prevent calling V8::EnableWebAssemblyTrapHandler * fix: no cppgc initialization in the renderer * gyp: put filenames in variables nodejs/node#46965 * test: disable single executable tests * fix: nan tests failing on node headers missing file * tls,http2: send fatal alert on ALPN mismatch nodejs/node#44031 * test: disable snapshot tests * nodejs/node#47887 * nodejs/node#49684 * nodejs/node#44193 * build: use deps/v8 for v8/tools Node.js hard depends on these in their builtins * test: fix edge snapshot stack traces nodejs/node#49659 * build: remove js2c //base dep * build: use electron_js2c_toolchain to build node_js2c * fix: don't create SafeSet outside packageResolve Fixes failure in parallel/test-require-delete-array-iterator: === release test-require-delete-array-iterator === Path: parallel/test-require-delete-array-iterator node:internal/per_context/primordials:426 constructor(i) { super(i); } // eslint-disable-line no-useless-constructor ^ TypeError: object is not iterable (cannot read property Symbol(Symbol.iterator)) at new Set (<anonymous>) at new SafeSet (node:internal/per_context/primordials:426:22) * fix: failing crashReporter tests on Linux These were failing because our change from node::InitializeNodeWithArgs to node::InitializeOncePerProcess meant that we now inadvertently called PlatformInit, which reset signal handling. This meant that our intentional crash function ElectronBindings::Crash no longer worked and the renderer process no longer crashed when process.crash() was called. We don't want to use Node.js' default signal handling in the renderer process, so we disable it by passing kNoDefaultSignalHandling to node::InitializeOncePerProcess. * build: only create cppgc heap on non-32 bit platforms * chore: clean up util:CompileAndCall * src: fix compatility with upcoming V8 12.1 APIs nodejs/node#50709 * fix: use thread_local BuiltinLoader * chore: fixup v8 patch indices --------- Co-authored-by: Keeley Hammond <vertedinde@electronjs.org> Co-authored-by: Samuel Attard <marshallofsound@electronjs.org>

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 9, 2022

joyeecheung commented Aug 9, 2022

View reviewed changes

mcollina approved these changes Aug 9, 2022

View reviewed changes

legendecas reviewed Aug 10, 2022

View reviewed changes

src/node_snapshotable.h Outdated Show resolved Hide resolved

joyeecheung mentioned this pull request Aug 10, 2022

Tracking issue: support of builtins and V8 issues in run-time user-land snapshots #44014

Open

10 tasks

legendecas mentioned this pull request Aug 10, 2022

src: iterate over base objects to prepare for snapshot #44192

Merged

legendecas approved these changes Aug 10, 2022

View reviewed changes

legendecas added the blocked PRs that are blocked by other issues or PRs. label Aug 10, 2022

joyeecheung mentioned this pull request Aug 17, 2022

Support Node.js embedder objects and bindings in the snapshot #37476

Open

joyeecheung added 2 commits August 18, 2022 22:30

src: support WeakReference in snapshot

7b165d9

Move util::WeakReference to a separate header and implement {de}serialization for it to be snapshotable.

src: support diagnostics channel in the snapshot

9fc1cd7

joyeecheung removed the blocked PRs that are blocked by other issues or PRs. label Aug 18, 2022

joyeecheung force-pushed the snapshot-channel branch from 8b5a4e4 to 9fc1cd7 Compare August 18, 2022 14:51

joyeecheung added a commit that referenced this pull request Aug 19, 2022

src: support diagnostics channel in the snapshot

fddc701

PR-URL: #44193 Refs: #44014 Refs: #37476 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

joyeecheung closed this Aug 19, 2022

ruyadorno added the backport-blocked-v18.x label Aug 22, 2022

This comment was marked as outdated.

Sign in to view

ruyadorno added backport-blocked-v18.x and removed backport-blocked-v18.x labels Aug 23, 2022

juanarbol added the backport-blocked-v16.x label Sep 30, 2022

RafaelGSS mentioned this pull request Oct 14, 2022

2022-10-18, Version 19.0.0 (Current) #44626

Merged

joyeecheung mentioned this pull request Oct 14, 2022

src: consolidate exit codes in the code base #44746

Merged

joyeecheung mentioned this pull request Oct 14, 2022

[v18.x] backport various patches of internal refactorings for snapshot and realms #45007

Closed

danielleadams mentioned this pull request Dec 30, 2022

v18.13.0 release proposal #46025

Merged

codebytere added a commit to electron/electron that referenced this pull request Jan 9, 2023

chore: remove errant semicolons

d44581f

- nodejs/node#44179 - nodejs/node#44193

codebytere added a commit to electron/electron that referenced this pull request Nov 17, 2023

test: disable snapshot tests

e4dfd3e

* nodejs/node#47887 * nodejs/node#49684 * nodejs/node#44193

codebytere added a commit to electron/electron that referenced this pull request Nov 21, 2023

test: disable snapshot tests

eb973d2

* nodejs/node#47887 * nodejs/node#49684 * nodejs/node#44193

codebytere added a commit to electron/electron that referenced this pull request Nov 22, 2023

test: disable snapshot tests

9ca751c

* nodejs/node#47887 * nodejs/node#49684 * nodejs/node#44193

codebytere added a commit to electron/electron that referenced this pull request Nov 28, 2023

test: disable snapshot tests

1325650

* nodejs/node#47887 * nodejs/node#49684 * nodejs/node#44193

codebytere added a commit to electron/electron that referenced this pull request Nov 29, 2023

test: disable snapshot tests

ce67ba1

* nodejs/node#47887 * nodejs/node#49684 * nodejs/node#44193

Uh oh!

Conversation

joyeecheung commented Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 9, 2022

Uh oh!

joyeecheung Aug 9, 2022

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Aug 9, 2022

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 10, 2022

Uh oh!

legendecas commented Aug 10, 2022

Uh oh!

joyeecheung commented Aug 11, 2022

Uh oh!

nodejs-github-bot commented Aug 18, 2022

Uh oh!

joyeecheung commented Aug 18, 2022

Uh oh!

joyeecheung commented Aug 19, 2022

Uh oh!

This comment was marked as outdated.

ruyadorno commented Aug 23, 2022

Uh oh!

juanarbol commented Sep 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

joyeecheung commented Aug 9, 2022 •

edited

Loading